Skip to content

chore: swallow TargetClosedErrors in beforeunload dialog.dismiss()#39788

Merged
dgozman merged 1 commit intomicrosoft:mainfrom
hbenl:dialog-dismiss-target-closed
Mar 25, 2026
Merged

chore: swallow TargetClosedErrors in beforeunload dialog.dismiss()#39788
dgozman merged 1 commit intomicrosoft:mainfrom
hbenl:dialog-dismiss-target-closed

Conversation

@hbenl
Copy link
Copy Markdown
Collaborator

@hbenl hbenl commented Mar 20, 2026

See #39701 (comment). This swallows TargetClosedErrors only for beforeunload dialogs because this test checks that dialog.dismiss() will throw for alert dialogs.

@hbenl hbenl force-pushed the dialog-dismiss-target-closed branch from 0554def to 5450e51 Compare March 20, 2026 13:06
@hbenl hbenl changed the title chore: swallow TargetClosedErrors in dialog.dismiss() chore: swallow TargetClosedErrors in beforeunload dialog.dismiss() Mar 20, 2026
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

try {
await this._channel.dismiss();
} catch (e) {
if (isTargetClosedError(e) && this.type() === 'beforeunload')
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the beforeunload?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because this test expects dialog.dismiss() to throw for an alert dialog if it was called after page.close().

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove this test, we think it is accidental, and allow safely dismissing any dialog.

@hbenl hbenl force-pushed the dialog-dismiss-target-closed branch from 5450e51 to 0ff7d51 Compare March 25, 2026 08:56
@github-actions
Copy link
Copy Markdown
Contributor

Test results for "tests 1"

2 failed
❌ [default] › debug-tests.spec.ts:516 › should not pause at the end of a setup test @vscode-extension
❌ [firefox-library] › library/overlay.spec.ts:135 › should show action highlight and title on click @firefox-ubuntu-22.04-node20

7 flaky ⚠️ [chromium-library] › library/trace-viewer.spec.ts:1223 › should display language-specific locators `@ubuntu-22.04-chromium-tip-of-tree`
⚠️ [chromium-library] › library/popup.spec.ts:261 › should not throw when click closes popup `@chromium-ubuntu-22.04-arm-node20`
⚠️ [chromium-library] › library/popup.spec.ts:261 › should not throw when click closes popup `@chromium-ubuntu-22.04-node24`
⚠️ [firefox-library] › library/browsercontext-reuse.spec.ts:116 › reuse launch › should reset serviceworker `@firefox-ubuntu-22.04-node20`
⚠️ [firefox-library] › library/inspector/cli-codegen-1.spec.ts:1080 › cli codegen › should not throw csp directive violation errors `@firefox-ubuntu-22.04-node20`
⚠️ [firefox-page] › page/page-wait-for-function.spec.ts:104 › should work with strict CSP policy `@firefox-ubuntu-22.04-node20`
⚠️ [playwright-test] › ui-mode-trace.spec.ts:812 › should update state on subsequent run `@macos-latest-node20`

38897 passed, 845 skipped


Merge workflow run.

@github-actions
Copy link
Copy Markdown
Contributor

Test results for "MCP"

5523 passed, 340 skipped


Merge workflow run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants